Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move BankHashStats from account's db to bank #3821

Merged
merged 18 commits into from
Dec 4, 2024
Merged

Move BankHashStats from account's db to bank #3821

merged 18 commits into from
Dec 4, 2024

Conversation

HaoranYi
Copy link

@HaoranYi HaoranYi commented Nov 27, 2024 •
edited
Loading

Problem

We keep track of bank_hash_stats for each bank. Currently, this stats live inside a locked hash map in Accounts DB. This cause a lock contention for every store of an accounts and hurts performance.

We want to get rid of the lock in the store code path.

Summary of Changes

Move bank_hash_stats from accounts db to bank.
Introduce AtomicBankHashStats to accumulate the stats in the bank.

Fixes #

@HaoranYi HaoranYi changed the title remove lock on BankHashStat in accounts-db store Remove lock on BankHashStat in accounts-db store Nov 27, 2024
@HaoranYi HaoranYi changed the title Remove lock on BankHashStat in accounts-db store Move BankHashStats from account's db to bank Nov 27, 2024
brooksprumo
brooksprumo reviewed Dec 2, 2024
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
@HaoranYi HaoranYi requested a review from brooksprumo December 2, 2024 21:04
brooksprumo
brooksprumo reviewed Dec 2, 2024
View reviewed changes
accounts-db/src/accounts_db/tests.rs Show resolved Hide resolved
runtime/src/snapshot_package.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/serde_snapshot.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Show resolved Hide resolved
brooksprumo
brooksprumo reviewed Dec 2, 2024
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
brooksprumo
brooksprumo reviewed Dec 2, 2024
View reviewed changes
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're missing updating the slot's bank_hash_stats in two other places:

  1. store_stake_accounts()
  2. commit_transactions()

Both of these functions call into accounts/accountsdb directly, so they bypass the Bank::store_accounts() logic.

@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024

Looks like we're missing updating the slot's bank_hash_stats in two other places:

  1. store_stake_accounts()
  2. commit_transactions()

Both of these functions call into accounts/accountsdb directly, so they bypass the Bank::store_accounts() logic.

done in 284e738

@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024

The log for BankHashStats matched with master when running on mainnet.

  • this pr
[2024-12-03T14:34:51.113908867Z INFO  solana_runtime::bank] bank frozen: 305167208 hash: 4qzkBXbkow8KTSQ3gpgwL7oGEQVmHkMFyxRJ2qSjzF63 accounts_delta: W2cZQQKRPkkx5gGaHjMKpTCvz6SNAjnLMKw2tXtmqoG signature_count: 2131 last_blockhash: CRSrNfrT2CMC32SxTZpXs65yx4ShaxyRHo9NASL3Ew93 capitalization: 589423807506551765, stats: BankHashStats { num_updated_accounts: 6359, num_removed_accounts: 233, num_lamports_stored: 17440095079623122, total_data_len: 39964086, num_executable_accounts: 3 }
[2024-12-03T14:34:51.533962317Z INFO  solana_runtime::bank] bank frozen: 305167209 hash: 3JFoM5ceBXWhrL41Hk2igBRe4s8Qq3JALFRGdGA77VJh accounts_delta: GyymD8hD8PYWiLtzUXZNbamY77pMzqYiNVNZYk3JPrg3 signature_count: 1136 last_blockhash: J9U3bY8CwwotGynEazzEN7MUC5vnsa15AgepuGY74sbe capitalization: 589423807419181024, stats: BankHashStats { num_updated_accounts: 3790, num_removed_accounts: 113, num_lamports_stored: 6829130698133182, total_data_len: 27757127, num_executable_accounts: 5 }
  • master
[2024-12-03T14:34:50.954047096Z INFO  solana_runtime::bank] bank frozen: 305167208 hash: 4qzkBXbkow8KTSQ3gpgwL7oGEQVmHkMFyxRJ2qSjzF63 accounts_delta: W2cZQQKRPkkx5gGaHjMKpTCvz6SNAjnLMKw2tXtmqoG signature_count: 2131 last_blockhash: CRSrNfrT2CMC32SxTZpXs65yx4ShaxyRHo9NASL3Ew93 capitalization: 589423807506551765, stats: BankHashStats { num_updated_accounts: 6359, num_removed_accounts: 233, num_lamports_stored: 17440095079623122, total_data_len: 39964086, num_executable_accounts: 3 }
[2024-12-03T14:34:51.365612394Z INFO  solana_runtime::bank] bank frozen: 305167209 hash: 3JFoM5ceBXWhrL41Hk2igBRe4s8Qq3JALFRGdGA77VJh accounts_delta: GyymD8hD8PYWiLtzUXZNbamY77pMzqYiNVNZYk3JPrg3 signature_count: 1136 last_blockhash: J9U3bY8CwwotGynEazzEN7MUC5vnsa15AgepuGY74sbe capitalization: 589423807419181024, stats: BankHashStats { num_updated_accounts: 3790, num_removed_accounts: 113, num_lamports_stored: 6829130698133182, total_data_len: 27757127, num_executable_accounts: 5 }

brooksprumo
brooksprumo reviewed Dec 3, 2024
View reviewed changes
Copy link

@brooksprumo brooksprumo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic looks correct now. We should test this against another node on mnb to ensure both produce the same bank hash stats, as these numbers are used to debug consensus mismatches.

Can you run master vs this PR and ensure the bank hash details that are logged in the "bank frozen" line are always the same? Ideally over an epoch boundary, since that'll exercise also storing the stake accounts.

Edit: Nice! I see you just did this test: #3821 (comment)

runtime/src/bank.rs Outdated Show resolved Hide resolved
@HaoranYi
Copy link
Author

HaoranYi commented Dec 3, 2024 •
edited
Loading

Yes. I will keep it running and check for the slot at the next epoch boundary, which is about 1d7h from now.

brooksprumo
brooksprumo approved these changes Dec 3, 2024
View reviewed changes
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Looks good to me! Probably best to wait and confirm the stats match master across an epoch boundary.

@HaoranYi
Copy link
Author

HaoranYi commented Dec 4, 2024

stats matched at the epoch boundary.

[2024-12-04T20:48:22.626606613Z INFO  solana_runtime::bank] bank frozen: 305424000 hash: GG83exajA53vbdsA8DPxougAybY6GPGsdT6eXkP4nFyT accounts_delta: H9uhPuch44MGrmzFUoHg4eBnZbF6pMbwhQF5nt84VCbb signature_count: 0 last_blockhash: oJ2wJEUjva6BU5uapEqDDZcnTwBEn1uRnx6vnLG56PN capitalization: 589453979955825477, stats: BankHashStats { num_updated_accounts: 1431, num_removed_accounts: 0, num_lamports_stored: 394203918046202, total_data_len: 5531172, num_executable_accounts: 0 }
[2024-12-04T20:48:25.022027026Z INFO  solana_runtime::bank] bank frozen: 305424001 hash: 5jt3dwBfJuYksn4UFctAjAUNwL9xwCbb3yiXmhijgd9 accounts_delta: FEJAMNFYjywEyqBtFkLcYkzDL6Jj5SwUNHbnaEFSsm78 signature_count: 28 last_blockhash: 7kxx1ChdRRVno6K5Vn1ANNAm2i8GvaBZa446HW3HMJXo capitalization: 589454371122902139, stats: BankHashStats { num_updated_accounts: 4311, num_removed_accounts: 5, num_lamports_stored: 1686863971165642, total_data_len: 1822705, num_executable_accounts: 0 }


[2024-12-04T20:48:22.385496541Z INFO  solana_runtime::bank] bank frozen: 305424000 hash: GG83exajA53vbdsA8DPxougAybY6GPGsdT6eXkP4nFyT accounts_delta: H9uhPuch44MGrmzFUoHg4eBnZbF6pMbwhQF5nt84VCbb signature_count: 0 last_blockhash: oJ2wJEUjva6BU5uapEqDDZcnTwBEn1uRnx6vnLG56PN capitalization: 589453979955825477, stats: BankHashStats { num_updated_accounts: 1431, num_removed_accounts: 0, num_lamports_stored: 394203918046202, total_data_len: 5531172, num_executable_accounts: 0 }
[2024-12-04T20:48:24.667013810Z INFO  solana_runtime::bank] bank frozen: 305424001 hash: 5jt3dwBfJuYksn4UFctAjAUNwL9xwCbb3yiXmhijgd9 accounts_delta: FEJAMNFYjywEyqBtFkLcYkzDL6Jj5SwUNHbnaEFSsm78 signature_count: 28 last_blockhash: 7kxx1ChdRRVno6K5Vn1ANNAm2i8GvaBZa446HW3HMJXo capitalization: 589454371122902139, stats: BankHashStats { num_updated_accounts: 4311, num_removed_accounts: 5, num_lamports_stored: 1686863971165642, total_data_len: 1822705, num_executable_accounts: 0 }

@jeffwashington jeffwashington self-requested a review December 4, 2024 21:31
jeffwashington
jeffwashington approved these changes Dec 4, 2024
View reviewed changes
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me. only used for debugging.

@HaoranYi HaoranYi merged commit c13ca2d into anza-xyz:master Dec 4, 2024
40 checks passed
@HaoranYi HaoranYi deleted the bank_stat_lock branch December 4, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffwashington jeffwashington jeffwashington approved these changes

@brooksprumo brooksprumo brooksprumo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HaoranYi @brooksprumo @jeffwashington